refactor: use the SDK's id validator instead of a local base58 check by lmjabreu · Pull Request #67 · Doist/comms-cli

lmjabreu · 2026-09-22T21:40:32Z

Overview

The id check added in #66 was a duplicate of the SDK's isValidUuidV7Base58, which I only spotted after it merged. This removes ours in favour of the SDK one.

Reference

Follows @scottlovegrove's review on #66. The error helpers from that PR (isNotFound, isConflict, isMalformedId) move to the SDK in Doist/comms-sdk-typescript#83, and this repo will pick them up once that's released.

Changelog

A token that looks like an id but isn't one, such as EngineeringDiscussion, is now refused locally with INVALID_REF rather than sent to Comms and refused there. Real ids behave as before.

Test plan

tdc channel threads <a channel id with no digit in it> --limit 1
- Lists that channel's threads
tdc channel archive <the same id> --dry-run
- [dry-run] Would archive channel naming that channel
tdc search x --channel <the same id> --limit 1
- Reaches the API, no INVALID_REF
tdc channel threads EngineeringDiscussion --limit 1
- Error: CHANNEL_NOT_FOUND: it decodes to 16 bytes but isn't a UUIDv7, so it stays a name
tdc conversation done <a conversation id with no digit in it> --dry-run
- [dry-run] Would archive conversation, the case Bare opaque ids without a digit are rejected, and bad ids surface as raw SDK errors #65 was about

Adversarial review

Reviewer	Head	Raised	Fixed	Refuted	Deferred	Reviewer tokens
Fable	`551ec77`	3	3	0	0	254,728

Doistbot: rounds and P2s filled in at merge.

🤖 Generated with Claude Code

`@doist/comms-sdk` has exported `isValidUuidV7Base58` since 0.11.1 (July), and it checks the v7 version nibble and variant bits on top of the base58 decode. The local `looksLikeOpaqueCommsId` checked neither, so a 21-char name like `EngineeringDiscussion` decoded to 16 bytes and read as an id. That collision is the only reason `resolveChannelRef` grew a name-first-then-getChannel fallback in #66, so both go: `getDirectChannelId` recognises an opaque id again, which also restores the workspace-agnostic behaviour a bare digit-free channel id had before #66. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

doistbot

Nice cleanup — swapping the local base58 check for the SDK's isValidUuidV7Base58 removes a duplicate we'd otherwise have to keep in sync with the SDK.

Few things worth tightening:

Removing the id fallback from resolveChannelRef breaks bare digit-free channel ids for its direct callers (channel threads, channel members list/add/remove/set, and channel update --workspace): a valid id like CbjxNkWHJBwcaVkoTCRgM is now parsed as a name and fails with CHANNEL_NOT_FOUND. Consider restoring the fallback there via isValidUuidV7Base58(parsed.name), or routing these commands through getDirectChannelId first.
On the new direct-ID path, prefer matching a channel name in the current workspace before treating an unprefixed, digit-free token as an id. Since such names are valid, workspace-agnostic getChannel lookup means tdc channel delete <name> --yes could target a channel with that name in another workspace if the user has permission there.

I also included a few optional follow-up notes in the details below.

Optional follow-up notes (4)

src/lib/refs.ts:355: getDirectChannelId now already applies getOpaqueNameId, so the identical fallback in resolveChannelId (lines 327-330, including the "no name to protect" comment) is unreachable. Delete that fallback and comment so the opaque-name check has a single owner.
src/lib/refs.test.ts:1041: Cf9TR6CPC2dKQL5fB2EoL doesn't exercise the SDK validator this block claims to cover. It contains digits, so parseRef returns it as type: 'id' via looksLikeRawId, and resolveConversationId returns it before getOpaqueNameId/isValidUuidV7Base58 runs. That means this case would still pass if the SDK delegation were removed, so it adds no regression signal here. Drop it from this list (or move it to a digit-path test) so the "delegated to the SDK validator" block only contains digit-free ids that actually reach isValidUuidV7Base58.
src/lib/refs.test.ts:1040: This block largely restates existing coverage: resolveConversationId('CDMDzXhBNCgyQZjkDnqwG') is already asserted in the resolveConversationId describe (line 663), and nope only exercises the trivial length rejection. The genuinely new signal is the valid-id-vs-16-byte-name distinction — trimming the block to one accepted id plus 'EngineeringDiscussion'/'CustomerSuccessLeadership' would keep that without the duplication.
src/lib/refs.test.ts:1037: The comment says "the look-alikes below decode to 16 bytes", but 'CustomerSuccessLeadership' is 25 characters (~19 bytes decoded) and 'nope' doesn't decode to 16 bytes either. Only 'EngineeringDiscussion' matches the claim, so the comment overstates what the data exercises — worth rewording so a future reader doesn't infer the validator keys on a 16-byte decode.

_{Share Feedback • Review Logs}

The previous commit moved bare-id recognition into getDirectChannelId and deleted the fallback from resolveChannelRef. Two regressions against 3.4.1: - channel threads, channel members and channel update --workspace call resolveChannelRef directly, so a bare digit-free channel id stopped resolving there at all. - channel archive, delete and update went id-first, through a workspace-agnostic getChannel, where 3.4.1 tried a name in the current workspace first. Back to 3.4.1's order in both places, with the SDK's validator in place of the local check. search and thread create keep resolving bare ids, since resolveChannelId has its own check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Found by mutation: letting the fallback fire on any name failure, not just CHANNEL_NOT_FOUND, passed every test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

lmjabreu · 2026-09-25T13:12:57Z

getDirectChannelId now already applies getOpaqueNameId, so the identical fallback in resolveChannelId […] is unreachable.

Keeping as-is. True of 7f5ff7b, but getDirectChannelId returns null for bare tokens again, so this is the check that keeps search --channel <bare id> working.

Cf9TR6CPC2dKQL5fB2EoL doesn't exercise the SDK validator this block claims to cover.

This block largely restates existing coverage

The comment says "the look-alikes below decode to 16 bytes", but 'CustomerSuccessLeadership' is 25 characters

Fixed all three in 551ec77. The block is down to the one contrast it's for: CbjxNkWHJBwcaVkoTCRgM against EngineeringDiscussion, both 21 digit-free characters that decode to 16 bytes.

Deleting the opaque-id fallback from resolveCommentId or resolveMessageId left the suite green; the other four call sites were already pinned. Also corrects two comments that still described the old decode-only check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

lmjabreu · 2026-09-25T15:08:04Z

@doistbot /review

doistbot

This PR swaps the local base58 id check for the SDK's isValidUuidV7Base58, removing the duplicate validator added in #66. The refactor is clean: getOpaqueNameId delegates to the SDK validator, the name-then-id fallback in resolveChannelRef keeps workspace membership enforcement intact, and the tests cover the accepted-id vs. 16-byte-name distinction well. No inline issues were flagged.

I also included a few optional follow-up notes in the details below.

Optional follow-up notes (2)

src/lib/refs.test.ts:377: getDirectChannelId returns null for any bare digit-free name without calling the SDK validator—even for the valid ID in the next test. This assertion cannot verify that the validator rejects EngineeringDiscussion. Remove the validator rationale here; the resolveConversationId test below covers that distinction.
src/lib/refs.test.ts:645: refuses an id that belongs to another workspace never asserts the id fallback actually ran. With an empty channel list, resolveChannelRef(id, 1) already rejects with CHANNEL_NOT_FOUND from matchByName, so this test passes unchanged if the fallback is removed or getOpaqueNameId stops recognizing CbjxNkWHJBwcaVkoTCRgM. The named behavior (fetching a cross-workspace channel via getChannel and refusing it) isn't pinned. Add expect(mockGetChannel).toHaveBeenCalledWith(id), matching the sibling it.each and fallback tests, so the workspace check is actually exercised.

_{Share Feedback • Review Logs}

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

lmjabreu · 2026-09-25T15:43:44Z

getDirectChannelId returns null for any bare digit-free name without calling the SDK validator

Fixed in 5349b9b. The comment now says every bare token goes to the name path there, and points at the resolveConversationId test for the id-vs-name case.

refuses an id that belongs to another workspace never asserts the id fallback actually ran.

Fixed in 5349b9b. Added expect(mockGetChannel).toHaveBeenCalledWith(id). With the fallback deleted it was green before and fails now.

lmjabreu mentioned this pull request Sep 22, 2026

fix: accept digit-free opaque ids and map 404/409 to CliError #66

Merged

8 tasks

lmjabreu marked this pull request as ready for review September 24, 2026 12:04

lmjabreu requested a review from scottlovegrove September 24, 2026 12:04

doistbot reviewed Sep 24, 2026

View reviewed changes

Comment thread src/lib/refs.ts Outdated

Comment thread src/lib/refs.ts Outdated

lmjabreu mentioned this pull request Sep 24, 2026

feat: add isNotFound, isConflict and isMalformedId error predicates Doist/comms-sdk-typescript#83

Open

lmjabreu and others added 4 commits September 25, 2026 13:49

test: pin that an ambiguous channel name never falls through to an id

c0148ae

Found by mutation: letting the fallback fire on any name failure, not just CHANNEL_NOT_FOUND, passed every test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

chore: keep the comment explaining the id-lookup fallback

aee5f1a

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

test: keep only the id-vs-look-alike case in the SDK validator block

551ec77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

scottlovegrove approved these changes Sep 25, 2026

View reviewed changes

doistbot reviewed Sep 25, 2026

View reviewed changes

test: make the cross-workspace fallback test see the fallback

5349b9b

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Navigation Menu

refactor: use the SDK's id validator instead of a local base58 check - #67

refactor: use the SDK's id validator instead of a local base58 check#67
lmjabreu wants to merge 7 commits into
mainfrom
lmjabreu/use-sdk-id-validator

lmjabreu commented Sep 22, 2026 •

edited

Loading

Uh oh!

doistbot left a comment

Uh oh!

Uh oh!

Uh oh!

lmjabreu commented Sep 25, 2026

Uh oh!

lmjabreu commented Sep 25, 2026

Uh oh!

doistbot left a comment

Uh oh!

lmjabreu commented Sep 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lmjabreu commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Reference

Changelog

Test plan

Adversarial review

Uh oh!

doistbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lmjabreu commented Sep 25, 2026

Uh oh!

lmjabreu commented Sep 25, 2026

Uh oh!

doistbot left a comment

Choose a reason for hiding this comment

Uh oh!

lmjabreu commented Sep 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lmjabreu commented Sep 22, 2026 •

edited

Loading